Fix multi-column column index rebuild ordering#899
Conversation
saulshanabrook
requested review from
FTRobbin and
yihozhang
and removed request for
a team and
FTRobbin
Merging this PR will improve performance by 27.43%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | rust_rule_fib[rule_run_1000] |
280.7 ms | 202.8 ms | +38.39% |
| 🆕 | Simulation | tests[repro-herbie-vanilla] |
N/A | 1.4 s | N/A |
| ⚡ | Simulation | tests[cykjson] |
440.2 ms | 270.9 ms | +62.51% |
| ❌ | Simulation | tests[python_array_optimize] |
1.5 s | 1.7 s | -7.99% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing saulshanabrook:codex/core-relations-index-order-fix (5740bec) with main (8c1c70b)2
Footnotes
-
215 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(2e1796e) during the generation of this report, so 8c1c70b was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #899 +/- ##
==========================================
+ Coverage 87.12% 87.16% +0.03%
==========================================
Files 88 88
Lines 25904 25905 +1
==========================================
+ Hits 22568 22579 +11
+ Misses 3336 3326 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
saulshanabrook
force-pushed
the
codex/core-relations-index-order-fix
branch
from
8276e21 to
5740bec
Compare
|
Closing in favor of #914 |
github-project-automation
Bot
moved this from Waiting for Review
to Done
in Saul Shanabrook
2 participants
Summary
Fixes a debug-only panic in
ColumnIndex::rebuild_fullfor multi-column indexes after a major table-version rebuild.The regression comes from #862, specifically commit
70677c590534ba06ecebdaa9f16ac3bb8b1c8a0c(cleanup). That commit switched multi-columnColumnIndex::rebuild_fullto the value-only radix sort path. Multi-column collection is column-major, so rows for the same value can be ordered like95..126, 0..30, 127;BufferedSubset::Sparsethen creates aSortedOffsetSlicefrom unsorted row IDs.Why this stayed hidden
The invariant violation is caught by a
debug_assert!incore-relations/src/offsets/mod.rs:136. Theegglog-experimentalCI job runsmake test, which usescargo nextest run --release, so that debug assertion is disabled in CI. In release mode, themath_backofftest passes even though the sorted-offset invariant is violated.This was investigated while updating egraphs-good/egglog-experimental#46, but it is not caused by that PR. A clean
egglog-experimentalorigin/maincheckout at1530719693f56c1186ab867629140c0db185783a, pinned to egg-smolcodex/split-scheduler-can-stop-report#ebba7bb902bdc1b0f377b6bb22c06ac305912674, already reproduces the same debug-modemath_backoffpanic incore-relations/src/offsets/mod.rs:136.So the timeline is:
egglog-experimentalpicked up an egg-smol commit containing that bug.egglog-experimentalCI stayed green because it runs release tests.Changes
ColumnIndex::rebuild_full, sort each equal-Valuegroup byRowIdbefore deduplicating.Validation
mainwithslice is not sorted.egglog-experimentalorigin/mainfailscargo test -q math_backoff --test files -- --exactin debug mode with the same upstream panic.egglog-experimentalorigin/mainpassescargo test --release -q math_backoff --test files -- --exact, matching CI's release-mode behavior.cargo fmt --checkcargo check -q -p egglog-core-relationscargo test -q -p egglog-core-relations --lib multi_column_column_index_rebuild_orders_each_value_by_rowcargo test -q -p egglog-core-relations --lib